Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

chore(package): add ESLint #600

Merged
merged 18 commits into from
Apr 24, 2019
Merged

chore(package): add ESLint #600

merged 18 commits into from
Apr 24, 2019

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Dec 11, 2018

This PR replaces ts-lint with eslint.

  • reduced linting time from >13s to ~3s 🎉
  • there is far more support and better plugins in the eslint community
  • we can now lint/fix JS files, which were getting ignored before by ts-lint

Changes

  • added ESLint with common plugins
  • disabled all failing rules, with uncomment them in future PRs

package.json Outdated
@@ -19,6 +19,8 @@
"deploy:docs": "gulp deploy:docs",
"lint": "tslint -t stylish -p .",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is now broken

package.json Outdated
@@ -19,6 +19,8 @@
"deploy:docs": "gulp deploy:docs",
"lint": "tslint -t stylish -p .",
"lint:fix": "yarn lint --fix",
"eslint": "eslint .",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to name this just lint. The fact that we use eslint for that is an implementation detail I don't care about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI, while the PR was in WIP state, I had both so I could run parallel tests of the two. Certainly agree on lint 👍

@kuzhelov kuzhelov added the needs author feedback Author's opinion is asked label Dec 12, 2018
@levithomason levithomason changed the title chore: replace ts-lint with eslint [WIP] chore: replace ts-lint with eslint Jan 11, 2019
@levithomason levithomason added 🚧 WIP and removed needs author feedback Author's opinion is asked labels Jan 11, 2019
@levithomason
Copy link
Member Author

This PR is blocked waiting on the latest release from eslint-typescript-parser. Our update to [email protected] is actually a breaking change for typescript-estree:

WARNING: You are currently running a version of TypeScript which is not officially
supported by typescript-estree.

SUPPORTED TYPESCRIPT VERSIONS: ~3.1.1
YOUR TYPESCRIPT VERSION: 3.2.2

See either of these issues:
eslint/typescript-eslint-parser#576
eslint/typescript-eslint-parser#589

@layershifter
Copy link
Member

Updated dependencies and unblocked PR 👍

@levithomason
Copy link
Member Author

I fixed the yarn.lock, but this caused issues with ESLint resolving the eslint config. Once fixing the resolution, there were over 3k lint errors. Will have to come back to this one.

@DustyTheBot
Copy link
Collaborator

Warnings
⚠️ There are no updates provided to CHANGELOG. Ensure there are no publicly visible changes introduced by this PR.
⚠️ Package (or peer) dependencies changed. Make sure you have approval before merging!

Changed dependencies in packages/internal-tooling/package.json

package before after
@typescript-eslint/eslint-plugin - ^1.6.0
@typescript-eslint/parser - ^1.6.0
eslint - ^5.16.0
eslint-config-airbnb - ^17.1.0
eslint-config-prettier - ^4.1.0
eslint-plugin-import - ^2.17.2
eslint-plugin-jest - ^22.4.1
eslint-plugin-jsx-a11y - ^6.2.1
eslint-plugin-prettier - ^3.0.1
eslint-plugin-react - ^7.12.4
eslint-plugin-react-hooks - ^1.6.0

Generated by 🚫 dangerJS

@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

Merging #600 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #600   +/-   ##
======================================
  Coverage    72.1%   72.1%           
======================================
  Files         731     731           
  Lines        5592    5592           
  Branches     1612    1612           
======================================
  Hits         4032    4032           
  Misses       1555    1555           
  Partials        5       5
Impacted Files Coverage Δ
packages/react/src/lib/whatInput.ts 65.47% <ø> (ø) ⬆️
packages/react-proptypes/src/leven.ts 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ba7100...e424023. Read the comment docs.

@layershifter layershifter changed the title [WIP] chore: replace ts-lint with eslint chore(package): replace ts-lint with eslint Apr 19, 2019
@@ -0,0 +1,4 @@
{
"extends": ["./packages/internal-tooling/eslint/index.js"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paths are not great, should be fixed in ESLint 6: eslint/eslint#11388

@@ -0,0 +1,5 @@
{
"rules": {
"import/no-unresolved": "off"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have requires for Enzyme, Simulant and other stuff that it's not in direct dependencies. Currently disabled

{
"files": "**/icons/*.tsx",
"rules": {
"react/prop-types": "off"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESLint with React plugin handles these icons as React components, it will be very strange to have there propTypes...

{
files: '**/jest.config.js',
rules: {
'global-require': 'off',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's safe to use require() there

@layershifter layershifter changed the title chore(package): replace ts-lint with eslint chore(package): add ESLint Apr 24, 2019
@layershifter
Copy link
Member

We decided to keep TSLint before at least important rules will work properly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants